Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support directives around attributes on "and" type #2631

Closed
wants to merge 8 commits into from

Conversation

jindraivanek
Copy link
Contributor

Fixes #628

Minimized repro:

#if NET2
[<Attr1>]
#else
[<Attr2>]
#endif
[<NoComparison>]
type internal T1 = T1
and [<NoComparison>]
#if NET2
[<Attr1>]
#else
[<Attr2>]
#endif
    internal T2 = T2

Main problem here was that directives after and keyword get linked to = as ContentBefore, and not priinted out.

This fix link directive trivia after and type + attribute combination to attribute node as ContentAfter.

Also changing the way how attributes are printed for and type, so trivias are actually printed out, and handling some newline issues in this case.

And fixing some related bugs, surfaced because this is probably first time when Directive is used as ContentAfter.

TODO

  • fix also for .fsi

@jindraivanek jindraivanek self-assigned this Nov 19, 2022
@nojaf
Copy link
Contributor

nojaf commented Nov 19, 2022

Hello, thanks for taking the time to spend some time on this.

At first glance, this PR is more about how we currently merge AttributeLists together in the genOnelinerAttributes where there is trivia involved that doesn't lead to accurate results.

Something like

type A = B
and [<X>][<Y>][<Z>] C = D

gets transformed to

type A = B
and [<X; Y; Z>] C = D

It is questionable if we should do this in the first place. The style guide doesn't explicitly state this and I believe Fantomas should not really transform this code.

If we processed each attribute list individually, the trivia would be restored accordingly.
And we could use a different formatting style when all lists don't fit on one line.

Something like:

type A = B
and
    [<X>] // comment makes it multiline
    [<Y>]
    [<Z>] 
    C = D

On a practical note, I'm reluctant to merge these changes as everything will change anyway in #2626.
That branch is about 87% complete so I prefer to tackle it over there instead.
As your PR would need to tackle v5.2 anyway and would be superseded by #2626 before the stable release anyway.

@jindraivanek
Copy link
Contributor Author

Hey, thanks for response.

Hello, thanks for taking the time to spend some time on this.

At first glance, this PR is more about how we currently merge AttributeLists together in the genOnelinerAttributes where there is trivia involved that doesn't lead to accurate results.

Something like

type A = B
and [<X>][<Y>][<Z>] C = D

gets transformed to

type A = B
and [<X; Y; Z>] C = D

Agreed, genOnelinerAttributes works this way.

It is questionable if we should do this in the first place. The style guide doesn't explicitly state this and I believe Fantomas should not really transform this code.

If we processed each attribute list individually, the trivia would be restored accordingly. And we could use a different formatting style when all lists don't fit on one line.

Something like:

type A = B
and
    [<X>] // comment makes it multiline
    [<Y>]
    [<Z>] 
    C = D

genAttributes should work this way.

Maybe we want to remove genOnelinerAttributes in favor of genAttributes?

On a practical note, I'm reluctant to merge these changes as everything will change anyway in #2626. That branch is about 87% complete so I prefer to tackle it over there instead. As your PR would need to tackle v5.2 anyway and would be superseded by #2626 before the stable release anyway.

Sounds good, #2626 moved forward during time I was working on this, and I want to look at it anyways. Wasn't aware that Project Dallas is so close to stable version :)

I am closing this PR, and revisit it for v5.2 when things will be ready for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional directives around internal keyword
2 participants